- 
                Notifications
    You must be signed in to change notification settings 
- Fork 212
operator: fix upgradeImages #1865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
acc52d9    to
    7d562f7      
    Compare
  
    | 
 Can you write how you expect it to work and how it works now. IIRC we decided SHA pinning is only for the env var approach. | 
| If image is  Before: For example, let's say there is 0.30.0 version of deployed operatorhub bundle using olm (as written in the link), and then let's say we deploy a qat plugin (0.30.0 version). Then, the qat-plugin is expected to be upgraded automatically according to the envVar, but it does not because the parsing has been wrong ( So, 
 Yeah, this fix is for fixing the misimplementation with env var approach. | 
7d562f7    to
    05ed072      
    Compare
  
    | Thanks for noticing this! TestUpgrade unit test needs updating to get this covered. Also .1 release is needed | 
| Yeah, I think we should have had it already... It's my mistake. | 
de64c4d    to
    663ba27      
    Compare
  
    663ba27    to
    d06711e      
    Compare
  
    d06711e    to
    f38b5d5      
    Compare
  
    f38b5d5    to
    bb0dcd1      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for full coverage, we'd also need a case where image is same as the corresponding envVar and when the envVar is not in the pinned/sha256 format.
| 
 Umm, we do not check in  So, do you want to improve the implementation? Or... did I misunderstand what you mean? | 
| 
 we can focus on the current functionality and make sure all scenarios are covered | 
bb0dcd1    to
    3ccb2fe      
    Compare
  
    | Fixed.! And,, added some comments in the code to make it understandable more easily (actually for myself to understand next time.. I need to always follow the process and understand again...). If you think it's not good, let me konw.! | 
Operatorhub bundle can have sha256 image tags that are put through env vars. When operator controller manager gets upgraded, its operands (plugin daemonsets) should be updated to the image in the env vars. But it has not been working properly because of wrong parsing. Fix it to parse the image names that have sha256 tags correctly so env vars in operator can be used as intended. Additionatlly, add comments with an example result to the part where parsing, trimming, or transforming the name of images happens in UpgradImages to make the process intuitive. Signed-off-by: Hyeongju Johannes Lee <[email protected]>
Signed-off-by: Hyeongju Johannes Lee <[email protected]>
3ccb2fe    to
    6915c7d      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
fixes: #1873
Operatorhub bundle can have sha256 image tags that are put through env vars. When operator controller manager gets upgraded, its operands (plugin daemonsets) should be updated to the image in the env vars. But it has not been working properly because of wrong parsing.
Fix it to parse the image names that have sha256 tags correctly so env vars in operator can be used as intended.